Skip to content

FIX Add support for scikit-learn 1.8#360

Merged
perimosocordiae merged 3 commits intoscikit-learn-contrib:masterfrom
dherrera1911:check-xy-fix
Mar 19, 2026
Merged

FIX Add support for scikit-learn 1.8#360
perimosocordiae merged 3 commits intoscikit-learn-contrib:masterfrom
dherrera1911:check-xy-fix

Conversation

@dherrera1911
Copy link
Contributor

@dherrera1911 dherrera1911 commented Mar 10, 2026

PR description at a high level:

Support scikit-learn 1.8, that introduced a breaking change. Maintain compatibility with versions <=1.7.

Fixes #359

Describe changes:

In scikit-learn 1.8, the parameter force_all_finite in the functions check_array() and check_X_y() was deprecated, and replaced for ensure_all_finite.

The function _utils.py::check_input() uses the parameter force_all_finite, making the package fail as described in #359.

In this PR, we introduce the local wrapper functions _check_array() and _check_X_y() inside of _utils.py. These wrappers work like their scikit-learn counterparts, but are able to take both force_all_finite and ensure_all_finite as inputs. Now _utils.py calls the wrappers instead of directly calling the functions.

Outstanding questions:

For transparency I mention that the original version of this PR was generated by a coding agent. After reviewing it, I concluded that the approach is sound. I did modify it considerably to make it simpler.

I have not tested this solution thoroughly, but I can run the relevant parts of the testing suite.

@dherrera1911
Copy link
Contributor Author

dherrera1911 commented Mar 10, 2026

I ran the tests in test/test_utils.py, and everything seems to be working good. Actually, the current version of metric-learn was throwing errors in these tests even before it broke, because of scikit-learn deprecation warnings.

For reference, this branch passes all tests with scikit-learn 1.8, while main fails a lot of tests.

Interestingly, when using scikit-learn 1.7, both this branch and main throw a bunch of errors saying e.g. FAILED test/test_utils.py::test_check_tuples_valid_tuple_size[2] - assert 4 == 0. This is because in both cases, the pattern below

  with warnings.catch_warnings(record=True) as record:
    check_input(tuples, type_of_inputs='tuples', preprocessor=None)
  assert len(record) == 0

I looked into one of these tests, and as I supposed, it was a FutureWarning from scikit-learn, warning about ensure_all_finite:

is catching `scikit-learn` warnings:

sklearn/utils/deprecation.py:132: FutureWarning: 'force_all_finite' was renamed to 'ensure_all_finite' in 1.6 and will be removed in 1.8.
warnings.warn(


In sum, it the current PR makes `metric-learn` work with `scikit-learn` 1.8, at least in the tests I ran and the use cases I tested for my own work.

@dherrera1911 dherrera1911 changed the title FIX TypeError bug from _util.py::check_input() FIX TypeError bug from scikit-learn 1.8 breaking change Mar 11, 2026
@dherrera1911 dherrera1911 changed the title FIX TypeError bug from scikit-learn 1.8 breaking change FIX Add support for scikit-learn 1.8 Mar 13, 2026
@perimosocordiae
Copy link
Contributor

Thanks! I had to manually enable the CI to run on this PR, but it started now.

else:
def vector_norm(X):
return np.linalg.norm(X, axis=1)
import numpy as np
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused why this line is showing up as part of the diff. Maybe a line-ending related change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. The file uses CRLF and I had accidentally changed it to LF. I converted the file to CRLF to remove this from the diff.

@dherrera1911
Copy link
Contributor Author

There are plenty of errors in the CI, but at least at a first approximation they don't seem to be cause by this PR.

For example, when running test/test_sklearn_compat.py locally with scikit-learn 1.8 and the current branch, I get the same errors as the CI. But I get even more errors with master, because of the issue this PR fixes. When using scikit-learn 1.7, I continue to get the exact same errors with both branches.

It is difficult to parse this CI output to try to identify issues with this PR when it's not clear (at least to me) what the CI looks like for master. But there do seem to be plenty of errors that require further investigation, not directly related to this PR. Maybe a larger effort to bring metric-learn up to date with scikit-learn is needed.

What do you think @perimosocordiae?

@dherrera1911
Copy link
Contributor Author

Also, I ran test/test_sklearn_compat.py with scikit-learn 1.5 and both branches pass all the tests. I didn't try 1.6, nor other testing suites yet because they take some time locally.

@perimosocordiae
Copy link
Contributor

Great, thanks for the contribution! (And sorry for the delay)

@perimosocordiae perimosocordiae merged commit 8eb775e into scikit-learn-contrib:master Mar 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TypeError: check_X_y() got an unexpected keyword argument 'force_all_finite'

2 participants